-
Notifications
You must be signed in to change notification settings - Fork 690
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
staging-xcm
: Use versioned_type!
for VersionedXcm
#4378
base: master
Are you sure you want to change the base?
Conversation
versioned_type!
for VersionedXcm
staging-xcm
: Use versioned_type!
for VersionedXcm
25162ca
to
8c56d43
Compare
8c56d43
to
924df33
Compare
924df33
to
37c8a6b
Compare
polkadot/xcm/src/lib.rs
Outdated
VersionedXcm::V2(x) | ||
} | ||
} | ||
|
||
impl<RuntimeCall> From<v3::Xcm<RuntimeCall>> for VersionedXcm<RuntimeCall> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this staying?
polkadot/xcm/src/lib.rs
Outdated
@@ -77,6 +77,37 @@ pub trait TryAs<T> { | |||
} | |||
|
|||
macro_rules! versioned_type { | |||
(@internal $n:ident, $v3:ty, ) => { | |||
// only impl `MaxEncodedLen` for enums without generic type parameters |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes no sense that this is separate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MaxEncodedLen
is not implemented for Xcm
structs of VersionedXcm
, so impl it fails. This is a workaround I did to exclude VersionedXcm
from it implementing MaxEncodedLen
. Open to suggestions for a more idiomatic way of handling it
polkadot/xcm/src/lib.rs
Outdated
// only impl `MaxEncodedLen` for enums without generic type parameters | ||
impl MaxEncodedLen for $n { | ||
fn max_encoded_len() -> usize { | ||
<$v3>::max_encoded_len() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be the max of all variants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some enums' 2nd versions do not implement MaxEncodedLen
, v3
and v4
all implement it, so I am now taking max of v3
and v4
Hope @franciscoaguirre can help with the macro review, I got lost there 🙈 |
@dastansam could you fix the CI issues? |
#[scale_info(replace_segment("staging_xcm", "xcm"))] | ||
pub enum VersionedXcm<RuntimeCall> { | ||
#[codec(index = 2)] | ||
#[deprecated] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this deprecated seems removed by the PR.
} | ||
impl MaxEncodedLen for $n { | ||
fn max_encoded_len() -> usize { | ||
<$v3>::max_encoded_len().max(<$v4>::max_encoded_len()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can also fix for the other implementation of MaxEncodedLen
when there is only v3 and v4:
https://github.com/paritytech/polkadot-sdk/pull/4378/files#diff-8eb727f0e7d0dd42a8d45ab05f9450777ed6af6d22021b81d0c449e8a32aba78R200
I also got lost in the macros, as it's been couple of months since I worked on it. I am getting familiar with it again and will also address PR comments from @gui1117 |
Co-authored-by: Adrian Catangiu <adrian@parity.io>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! We probably only need to make the CI happy in order to merge it
- audience: Runtime Dev | ||
description: | | ||
Currently, all other versioned types in the `staging-xcm` are created using `versioned_type!` macro, except for `VersionedXcm`. | ||
This PR changes `versioned_type` macro so that it can be used for `VersionedXcm` as well. This is done by adding optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR changes `versioned_type` macro so that it can be used for `VersionedXcm` as well. This is done by adding optional | |
This PR changes `versioned_type` macro so that it can be used for `VersionedXcm` as well, reducing a lot of duplication. This is done by adding optional |
@@ -476,66 +469,6 @@ impl<C> VersionedXcm<C> { | |||
} | |||
} | |||
|
|||
impl<RuntimeCall> From<v2::Xcm<RuntimeCall>> for VersionedXcm<RuntimeCall> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beautiful
The CI pipeline was cancelled due to failure one of the required jobs. |
closes #3846